Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Table - cell merging #4046

Merged
merged 8 commits into from
Mar 8, 2023
Merged

Table - cell merging #4046

merged 8 commits into from
Mar 8, 2023

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented Mar 7, 2023

This PR adds support for cell merging on the stable tables implementation. We store this with a simple __colSpan and __rowSpan property on the GridCellNode.

There are many follow ups from this PR, including the fact that only basic cases work for now:

  • Doesn't copy merged content onto the merged cell
  • Rewrite getShape() - indexes fail when merged cells are present
  • Fix paste (it's already crashing in the current version for some reason)
  • Kill GridSelection - performance and budget issues (adds cost the Lexical Core plain text users that don't use tables)

Cell merging

Screen.Recording.2023-03-07.at.4.49.26.PM.mov

(yes, the menu visibility needs a fix but we're considering dropping it for a better a11y alternative)

Selection is guaranteed to be rectangular

Screen.Recording.2023-03-07.at.2.58.29.PM.mov

More complicated selection

Screen.Recording.2023-03-07.at.3.13.29.PM.mov

@vercel
Copy link

vercel bot commented Mar 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
lexical ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 8, 2023 at 5:09PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 8, 2023 at 5:09PM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2023
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 26.72 KB (+2.53% 🔺) 535 ms (+2.53% 🔺) 114 ms (+46.34% 🔺) 648 ms
packages/lexical-rich-text/dist/LexicalRichText.js 37.48 KB (+1.65% 🔺) 750 ms (+1.65% 🔺) 151 ms (+47.93% 🔺) 900 ms
packages/lexical-plain-text/dist/LexicalPlainText.js 37.45 KB (+1.66% 🔺) 750 ms (+1.66% 🔺) 95 ms (-48.43% 🔽) 844 ms

Copy link
Contributor

@thegreatercurve thegreatercurve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me but @tylerjbainbridge should probably have a look. @zurfyx Maybe add a couple of test cases for showing what happens when you insert/delete a column or row which contains or insersects one with a merged cell.


type TableCellActionMenuProps = Readonly<{
contextRef: {current: null | HTMLElement};
onClose: () => void;
setIsMenuOpen: (isOpen: boolean) => void;
tableCellNode: TableCellNode;
cellMerge__EXPERIMENTAL: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to mark this as experimental if this is just in the playground. We'll just end up forgetting about it anyway

Suggested change
cellMerge__EXPERIMENTAL: boolean;
cellMerge boolean;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants